Just some random questions:

1. Does it make sense that Native Java objects are modifiable by default / at all?
2. Does this PR play nice with sealed (shared) scopes?

@rPraml as you've done a lot in the area of Java interop: any thoughts on this PR?

Am also wondering how this PR related to #1085 (if at all)

1. Does it make sense that Native Java objects are modifiable by default / at all?

At the moment they are "modifiable" in the sense that you can do o.property = value if there exists a public field named property. It is not possible however to add or remove properties.
For our use cases at ServiceNow, we rely a lot on the fact that they are modifiable.

2. Does this PR play nice with sealed (shared) scopes?

Not sure what that is; can you give me some pointers?

Am also wondering how this PR related to #1085 (if at all)

Not sure, but for NativeObject we do not define "properly" the prototype chain anyway - if you have a NativeJavaObject mapping, say, a java.lang.List, you don't have a prototype that points to something that maps the java class. So I think there is no conflict, but I haven't checked. Also, that PR has been stuck for over two years...

Info about sealed (shared) scopes be be found here

#1085 has been stuck indeed, but that is due to not having a clear-cut direction to proceed:

• besides the Object.freeze/seal JavaScript api's, on the Java level Rhino has a somewhat alternative mechanism to seal scopes. This mechanism predates the Object.seal/freeze additions to JavaScript
• Should ScriptableObject.sealObject also prevent modifications of parent and prototype #1085 is trying to fix a perceived bug in that implementation
• In EcmaScript, there are several proposals to provide secure EcmaScript (SES), where properly freezing everything, including prototype objects is an imporant part
• in fix #1300 Support ES2019 Function.toString() #1537 is was mentioned that some of the 'íntrospection' apis of JavaScript dont work on NativeJavaXxxx host objects (summary here
• the above issue is likely partly due to NativeJavaXxxx host objects just implementing Scriptable, while stuff related to PropertyDescriptors is implemented in ScriptableObject (I'm assuming here)

Given all of the above, I'm not sure if the approach taken in this PR is the best approach to take for the long term. Like to get some other opinions...

At the moment they are "modifiable" in the sense that you can do o.property = value if there exists a public field named property

Assume this also goes for there being a public getter/setter pair on the Java class?

• besides the Object.freeze/seal JavaScript api's, on the Java level Rhino has a somewhat alternative mechanism to seal scopes. This mechanism predates the Object.seal/freeze additions to JavaScript

This PR implements also a java API to freeze a NativeJavaObject, which goes into that direction. It plays well with scopes on which one calls ScriptableObject::sealObject.

• Should ScriptableObject.sealObject also prevent modifications of parent and prototype #1085 is trying to fix a perceived bug in that implementation
• In EcmaScript, there are several proposals to provide secure EcmaScript (SES), where properly freezing everything, including prototype objects is an imporant part

That doesn't really apply to this PR, because this one only touches NativeJavaXxxx, which have no parent or prototype.

• in fix #1300 Support ES2019 Function.toString() #1537 is was mentioned that some of the 'íntrospection' apis of JavaScript dont work on NativeJavaXxxx host objects (summary here
• the above issue is likely partly due to NativeJavaXxxx host objects just implementing Scriptable, while stuff related to PropertyDescriptors is implemented in ScriptableObject (I'm assuming here)

Yes, I know they don't work. These objects also don't have parent or prototype. So, while you cannot mark a single property as writable: false, with these changes you can still mark them as frozen, which is an improvement.

Given all of the above, I'm not sure if the approach taken in this PR is the best approach to take for the long term. Like to get some other opinions...

IMHO this PR is not trying to be a big revolutionary change in freezing stuff. It's just a small enhancement to allow to also freeze the NativeJavaXxxx objects, and that's it.

At the moment they are "modifiable" in the sense that you can do o.property = value if there exists a public field named property

Assume this also goes for there being a public getter/setter pair on the Java class?

Yeah, or a public field. And you cannot mutate methods.

Related to the getter/setter methods, I don't think it's possible to prevent called java methods from mutating fields. A javascript function that performs this.property = value will eventually call the put operation on the this object, which will check if this is frozen.

But a java method will mutate fields as it sees fit, even if the NativeJavaObject wrapping the object has been frozen. In your test case that looks like this,

var l = new Packages.java.util.ArrayList()
l.add('abc')
Object.freeze(l);
l[0] = 'def';

What is preventing someone from doing this after the object is frozen?

l.set(0, 'def')

I'm not sure if it should mess with NativeJavaMap either. Freezing an object which is an instance of a javascript Map does not prevent you from setting values in the map, it only prevents you from setting properties on the object.

With a javascript Map,

var m = new Map()
m['a'] = 1 // sets a property on the object
m.set('a', 2) // assigns a value to a key in the map
Object.freeze(m)
m['a'] = 3
m.set('a', 4)
assert(m['a'] === 1) // because the object was frozen when attempting to set to 3
assert(m.get('a') === 4) // not affected by the freeze

Your test case requires that Context.FEATURE_ENABLE_JAVA_MAP_ACCESS is enabled. Because this feature translates property access to Map access, your code tries to prevent setting values in the map (which can be circumvented by calling m.put(k, v)) As you pointed out, it's already not possible to set arbitrary property values on a NativeJavaObject like you can on most other Scriptables.

I'm sure someone could also get around the measures you are putting in place for actual fields using java reflection.

I'm curious what the use case is for wanting to freeze a java object?

But a java method will mutate fields as it sees fit, even if the NativeJavaObject wrapping the object has been frozen. In your test case that looks like this,

var l = new Packages.java.util.ArrayList()
l.add('abc')
Object.freeze(l);
l[0] = 'def';

What is preventing someone from doing this after the object is frozen?

l.set(0, 'def')

In our case, we don't have setter, but public fields. This PR fixes mutations of them.

I'm not sure if it should mess with NativeJavaMap either. Freezing an object which is an instance of a javascript Map does not prevent you from setting values in the map, it only prevents you from setting properties on the object.

With a javascript Map,

var m = new Map()
m['a'] = 1 // sets a property on the object
m.set('a', 2) // assigns a value to a key in the map
Object.freeze(m)
m['a'] = 3
m.set('a', 4)
assert(m['a'] === 1) // because the object was frozen when attempting to set to 3
assert(m.get('a') === 4) // not affected by the freeze

Your test case requires that Context.FEATURE_ENABLE_JAVA_MAP_ACCESS is enabled. Because this feature translates property access to Map access, your code tries to prevent setting values in the map (which can be circumvented by calling m.put(k, v)) As you pointed out, it's already not possible to set arbitrary property values on a NativeJavaObject like you can on most other Scriptables.

Agreed, there's not much I think we can do about this.

I'm curious what the use case is for wanting to freeze a java object?

We have a sandbox environment where we want to prevent modifications to the java objects that we expose (because some clever people have found security problems by mutating them).

I think I have a problem with the NativeJavaMap changes since the part of the object that you are preventing from mutating should still be mutable, even when frozen, using a javascript Map for comparable behavior. It's easy enough to wrap your java Maps using java.util.Collections.unmodifiableMap(m) before passing them into the Rhino environment if you don't want them to be modified.

I think the NativeJavaArray and NativeJavaList changes are ok, since there is no distinction between an index and a named property in a javascript array.

The rest of the changes are probably fine, but I don't think they'll actually solve your problem. Most java classes are written with public accessor and mutator methods and private fields, so they'll bypass the freeze without even trying. A motivated user with access to java can also use java reflection to set public and private fields.

If you have specific objects to which you are giving the user access which you want to protect, the best solution I can think of would be to create custom Scriptable objects which wrap those java objects and provide a public API for your users so they don't have direct access to them.

I am on the fence about putting in changes that "freeze" java objects, when it is clearly only a half-measure which is trivial to circumvent, and it is impossible to fully freeze them.

@rPraml any thoughts on this one, given that you/your team seems to use Java Interop-related in Rhino a lot

Like @tonygermano I'm a bit on the fence on this one as well

It sounds like you control the the code to some extent of you would be able to call freeze on the NativeJavaObject instances before an untrusted (user-provided) js code wood be able to interact with it

If so, other strategies may be better, like instead of direct access to Java classes, just expose custom wrappers, existing just those methods/properties you want exposed. Or make the Java classes you want your users to have access to to implement Scriptable and use annotations to tell Rhino what to expose and how

How feasible the suggestions are in your Rhino integration I cannot know, but given the limitations of the proposed freezing option, it might be a more viable way forward

Sorry, I missed the mention.

I'll try to summarize, what we have now:

1. On the Javascript side, there is Object.freeze() defined in the spec here. And there is Object.seal() here.

The difference between freeze and seal is, that seal allows to write to existing properties, as long as they are writeable.

Both do currently only work on ScriptableObjects, if language level is at least EcmaScript 6.
They also may work only properly in strict mode.
sealing and freezing can be done in JavaScript code. There is no (simple) java API to seal/freeze scriptables.

2. On the Java side there is ScriptableObject.sealObject. (See Should ScriptableObject.sealObject also prevent modifications of parent and prototype #1085) At least we use it, to protect objects in a shared scope, so that no script can modify or abuse these objects. They always fail (also in non strict mode) For NativeJavaList/Map we used unmodifiable lists/maps, but I'm not opposed against your approach.

I'm unsure if Object.freeze() / Object.seal() and ScriptableObject.sealObject are different things. If we really decide, these are different things for different useCases (See #1085), I would say, if you rename NativeJavaObject.freezeObject to sealObject and always fail, then the code would be more logical to the existing mechanism.

Can you describe your exact use case? Do you want to protect objects passed into the rhino engine?